Merge https://github.com/vmware-tanzu/velero:main (bb9a94b) into oadp-dev#483
Open
oadp-rebasebot-app[bot] wants to merge 63 commits intoopenshift:oadp-devfrom
Open
Merge https://github.com/vmware-tanzu/velero:main (bb9a94b) into oadp-dev#483oadp-rebasebot-app[bot] wants to merge 63 commits intoopenshift:oadp-devfrom
oadp-rebasebot-app[bot] wants to merge 63 commits intoopenshift:oadp-devfrom
Conversation
Signed-off-by: dongqingcc <dongqingcc@vmware.com>
…estore exec hooks. Signed-off-by: dongqingcc <dongqingcc@vmware.com>
Signed-off-by: dongqingcc <dongqingcc@vmware.com>
Revert PR 6105. Signed-off-by: Xun Jiang <xun.jiang@broadcom.com>
…-PR9366 Add e2e test case for PR 9366
…mware-tanzu#9581) * Fix DBR stuck when CSI snapshot no longer exists in cloud provider During backup deletion, VolumeSnapshotContentDeleteItemAction creates a new VSC with the snapshot handle from the backup and polls for readiness. If the underlying snapshot no longer exists (e.g., deleted externally), the CSI driver reports Status.Error but checkVSCReadiness() only checks ReadyToUse, causing it to poll for the full 10-minute timeout instead of failing fast. Additionally, the newly created VSC is never cleaned up on failure, leaving orphaned resources in the cluster. This commit: - Adds Status.Error detection in checkVSCReadiness() to fail immediately on permanent CSI driver errors (e.g., InvalidSnapshot.NotFound) - Cleans up the dangling VSC when readiness polling fails Fixes vmware-tanzu#9579 Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> * Add changelog for PR vmware-tanzu#9581 Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> * Fix typo in pod_volume_test.go: colume -> volume Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> --------- Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
The itemOperationTimeout field was missing from the Schedule API type documentation even though it is supported in the Schedule CRD template. This led users to believe the field was not available per-schedule. Fixes vmware-tanzu#9598 Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
…disable_search_in_site Disable Algolia docs search
WalkthroughAdds fast-fail and cleanup for CSI VolumeSnapshotContent readiness, extends unit tests, removes Algolia/DocSearch site integration, documents Schedule.spec.template.itemOperationTimeout, adds two E2E tests (RestoreExecHooks, WildcardNamespaces) and small test refactors plus dependency bumps. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
requested review from
savitharaghunathan and
shubham-pampattiwar
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @oadp-rebasebot-app[bot]. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/delete/actions/csi/volumesnapshotcontent_action_test.go (2)
97-109: Test coverage could be more explicit about cleanup verification.The test case validates that an error is returned when CSI reports an error, but it doesn't explicitly verify that the dangling VSC was cleaned up (deleted). Consider adding an assertion to check that the VSC no longer exists in the fake client after
Executereturns, to fully validate the cleanup behavior described in the test name.💡 Suggested enhancement to verify cleanup
After
p.Execute(...)returns, you could add:// Verify VSC was cleaned up vscList := &snapshotv1api.VolumeSnapshotContentList{} require.NoError(t, crClient.List(context.Background(), vscList)) require.Empty(t, vscList.Items, "Expected VSC to be cleaned up")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/delete/actions/csi/volumesnapshotcontent_action_test.go` around lines 97 - 109, The test should explicitly verify that the dangling VolumeSnapshotContent was deleted after Execute; after calling p.Execute(...) use the test client (crClient) to List VolumeSnapshotContent objects (snapshotv1api.VolumeSnapshotContentList) and assert no items remain by calling require.NoError for the List result and require.Empty on the list.Items (referencing the existing vsc variable and the Execute call) so the test confirms cleanup behavior rather than only checking expectErr.
242-248: Consider reusing existing pointer utilities.The codebase already has
pkg/util/boolptr(used in the main implementation file). For consistency, consider reusing that package forboolPtr. ForstringPtr, this local helper is acceptable since there doesn't appear to be a corresponding utility in the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/delete/actions/csi/volumesnapshotcontent_action_test.go` around lines 242 - 248, The local test helper boolPtr should be replaced with the existing utility to ensure consistency: remove the local boolPtr function and use the package pkg/util/boolptr (used in the implementation) wherever boolPtr() is referenced in this test (e.g., in volumesnapshotcontent_action_test.go); keep the local stringPtr helper as-is since no shared string pointer utility exists. Ensure the import for the util boolptr package is added and update references from boolPtr(...) to the util's API.test/e2e/basic/restore_exec_hooks.go (1)
51-54: Unusual format string for namespace numbering.The format
%00000dis unusual and likely not producing the intended result. The sequence00000means the minimum width is 0 (the rightmost digit), so there's no zero-padding. If the intent is 5-digit zero-padded numbers, use%05dinstead.💡 Suggested fix
for nsNum := 0; nsNum < r.NamespacesTotal; nsNum++ { - createNSName := fmt.Sprintf("%s-%00000d", r.CaseBaseName, nsNum) + createNSName := fmt.Sprintf("%s-%05d", r.CaseBaseName, nsNum) *r.NSIncluded = append(*r.NSIncluded, createNSName) }Note: This also appears at lines 78 and 142 in
CreateResources()andVerify().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/basic/restore_exec_hooks.go` around lines 51 - 54, The namespace name formatting in the loop that builds createNSName uses the incorrect format string "%00000d" which doesn't produce zero-padded numbers; change it to "%05d" in the loop that constructs createNSName (inside CreateResources()/the namespace-building loop) and update the other occurrences of the same format (the instances referenced in Verify() and the second CreateResources() occurrence) to use "%05d" so namespace numbers are zero-padded to 5 digits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@site/content/docs/main/api-types/schedule.md`:
- Around line 66-69: Update the grammatical errors in the documentation values:
change "The default value is 4 hour." to "The default value is 4 hours." for the
itemOperationTimeout entry (referenced by the symbol itemOperationTimeout) and
also change "10 minute" to "10 minutes" in the related schedule/config entry
mentioned earlier (ensure the corresponding field name remains correct in the
same doc), keeping plural "minutes" and "hours" for the default value
descriptions.
---
Nitpick comments:
In `@internal/delete/actions/csi/volumesnapshotcontent_action_test.go`:
- Around line 97-109: The test should explicitly verify that the dangling
VolumeSnapshotContent was deleted after Execute; after calling p.Execute(...)
use the test client (crClient) to List VolumeSnapshotContent objects
(snapshotv1api.VolumeSnapshotContentList) and assert no items remain by calling
require.NoError for the List result and require.Empty on the list.Items
(referencing the existing vsc variable and the Execute call) so the test
confirms cleanup behavior rather than only checking expectErr.
- Around line 242-248: The local test helper boolPtr should be replaced with the
existing utility to ensure consistency: remove the local boolPtr function and
use the package pkg/util/boolptr (used in the implementation) wherever boolPtr()
is referenced in this test (e.g., in volumesnapshotcontent_action_test.go); keep
the local stringPtr helper as-is since no shared string pointer utility exists.
Ensure the import for the util boolptr package is added and update references
from boolPtr(...) to the util's API.
In `@test/e2e/basic/restore_exec_hooks.go`:
- Around line 51-54: The namespace name formatting in the loop that builds
createNSName uses the incorrect format string "%00000d" which doesn't produce
zero-padded numbers; change it to "%05d" in the loop that constructs
createNSName (inside CreateResources()/the namespace-building loop) and update
the other occurrences of the same format (the instances referenced in Verify()
and the second CreateResources() occurrence) to use "%05d" so namespace numbers
are zero-padded to 5 digits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9dacb3df-e509-4af3-8614-195975c0171b
📒 Files selected for processing (9)
changelogs/unreleased/9581-shubham-pampattiwarinternal/delete/actions/csi/volumesnapshotcontent_action.gointernal/delete/actions/csi/volumesnapshotcontent_action_test.gosite/algolia-crawler.jsonsite/content/docs/main/api-types/schedule.mdsite/layouts/docs/docs.htmlsite/layouts/partials/head-docs.htmltest/e2e/basic/restore_exec_hooks.gotest/e2e/e2e_suite_test.go
💤 Files with no reviewable changes (3)
- site/layouts/docs/docs.html
- site/layouts/partials/head-docs.html
- site/algolia-crawler.json
Comment on lines
+66
to
+69
| # ItemOperationTimeout specifies the time used to wait for | ||
| # asynchronous BackupItemAction operations | ||
| # The default value is 4 hour. | ||
| itemOperationTimeout: 4h |
There was a problem hiding this comment.
Fix grammatical error in documentation.
Line 68 contains a grammatical error: "The default value is 4 hour." should be "The default value is 4 hours." (plural).
Note: The same grammatical issue exists in line 65 ("10 minute" should be "10 minutes"). Consider fixing both for consistency.
📝 Proposed fix
# ItemOperationTimeout specifies the time used to wait for
# asynchronous BackupItemAction operations
- # The default value is 4 hour.
+ # The default value is 4 hours.
itemOperationTimeout: 4hIf fixing line 65 as well:
# CSISnapshotTimeout specifies the time used to wait for
# CSI VolumeSnapshot status turns to ReadyToUse during creation, before
- # returning error as timeout. The default value is 10 minute.
+ # returning error as timeout. The default value is 10 minutes.
csiSnapshotTimeout: 10m📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # ItemOperationTimeout specifies the time used to wait for | |
| # asynchronous BackupItemAction operations | |
| # The default value is 4 hour. | |
| itemOperationTimeout: 4h | |
| # ItemOperationTimeout specifies the time used to wait for | |
| # asynchronous BackupItemAction operations | |
| # The default value is 4 hours. | |
| itemOperationTimeout: 4h |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@site/content/docs/main/api-types/schedule.md` around lines 66 - 69, Update
the grammatical errors in the documentation values: change "The default value is
4 hour." to "The default value is 4 hours." for the itemOperationTimeout entry
(referenced by the symbol itemOperationTimeout) and also change "10 minute" to
"10 minutes" in the related schedule/config entry mentioned earlier (ensure the
corresponding field name remains correct in the same doc), keeping plural
"minutes" and "hours" for the default value descriptions.
sseago
approved these changes
Mar 12, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: oadp-rebasebot-app[bot], sseago The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
From 1.18.1, Velero adds some default affinity in the backup/restore pod, so we can't directly compare the whole affinity, but we can verify if the expected affinity is contained in the pod affinity. Signed-off-by: Xun Jiang <xun.jiang@broadcom.com>
…r-PR9255 Add e2e test case for PR 9255
oadp-rebasebot-app
bot
changed the title
oadp-rebasebot-cloner
bot
force-pushed
the
rebase-bot-oadp-dev
branch
from
3e35877 to
26e68c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/delete/actions/csi/volumesnapshotcontent_action_test.go`:
- Around line 97-109: The test never asserts the expectErr==true case and it
globally overrides the checkVSCReadiness stub without restoring it, which breaks
isolation; update the table-driven test in volumesnapshotcontent_action_test.go
so each case checks both success and failure paths (assert when expectErr is
true as well as false) and ensure any override of the package-level
checkVSCReadiness function is saved and restored (e.g., save original :=
checkVSCReadiness and defer restoring it) around the test case where function is
replaced; reference the table entry fields expectErr and function, and the
global symbol checkVSCReadiness (and TestCheckVSCReadiness) when making these
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db69be39-38dd-4fad-88ca-3a059b6b5379
📒 Files selected for processing (10)
changelogs/unreleased/9581-shubham-pampattiwarinternal/delete/actions/csi/volumesnapshotcontent_action.gointernal/delete/actions/csi/volumesnapshotcontent_action_test.gosite/algolia-crawler.jsonsite/content/docs/main/api-types/schedule.mdsite/layouts/docs/docs.htmlsite/layouts/partials/head-docs.htmltest/e2e/basic/restore_exec_hooks.gotest/e2e/e2e_suite_test.gotest/e2e/resource-filtering/wildcard_namespaces.go
💤 Files with no reviewable changes (3)
- site/layouts/partials/head-docs.html
- site/layouts/docs/docs.html
- site/algolia-crawler.json
✅ Files skipped from review due to trivial changes (1)
- changelogs/unreleased/9581-shubham-pampattiwar
🚧 Files skipped from review as they are similar to previous changes (2)
- site/content/docs/main/api-types/schedule.md
- test/e2e/basic/restore_exec_hooks.go
Comment on lines
+97
to
+109
| { | ||
| name: "Error case with CSI error, dangling VSC should be cleaned up", | ||
| vsc: builder.ForVolumeSnapshotContent("bar").ObjectMeta(builder.WithLabelsMap(map[string]string{velerov1api.BackupNameLabel: "backup"})).Status(&snapshotv1api.VolumeSnapshotContentStatus{SnapshotHandle: &snapshotHandleStr}).Result(), | ||
| backup: builder.ForBackup("velero", "backup").ObjectMeta(builder.WithAnnotationsMap(map[string]string{velerov1api.ResourceTimeoutAnnotation: "5s"})).Result(), | ||
| expectErr: true, | ||
| function: func( | ||
| ctx context.Context, | ||
| vsc *snapshotv1api.VolumeSnapshotContent, | ||
| client crclient.Client, | ||
| ) (bool, error) { | ||
| return false, errors.Errorf("VolumeSnapshotContent %s has error: InvalidSnapshot.NotFound", vsc.Name) | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Test assertions and global stub isolation are currently broken.
Two issues weaken the new coverage:
expectErr: truepaths are never asserted (onlyexpectErr == falseis checked at Line 133-Line 135).checkVSCReadinessis globally overridden at Line 116 and not restored, soTestCheckVSCReadinesscan execute a leftover stub instead of the real implementation.
This can let the new fast-fail/cleanup cases pass without actually validating behavior.
✅ Suggested fix
func TestVSCExecute(t *testing.T) {
@@
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
crClient := velerotest.NewFakeControllerRuntimeClient(t)
logger := logrus.StandardLogger()
- checkVSCReadiness = test.function
+ if test.function != nil {
+ originalCheck := checkVSCReadiness
+ checkVSCReadiness = test.function
+ t.Cleanup(func() {
+ checkVSCReadiness = originalCheck
+ })
+ }
@@
- if test.expectErr == false {
- require.NoError(t, err)
- }
+ if test.expectErr {
+ require.Error(t, err)
+ } else {
+ require.NoError(t, err)
+ }
})
}
}Also applies to: 116-117, 133-136, 206-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/delete/actions/csi/volumesnapshotcontent_action_test.go` around
lines 97 - 109, The test never asserts the expectErr==true case and it globally
overrides the checkVSCReadiness stub without restoring it, which breaks
isolation; update the table-driven test in volumesnapshotcontent_action_test.go
so each case checks both success and failure paths (assert when expectErr is
true as well as false) and ensure any override of the package-level
checkVSCReadiness function is saved and restored (e.g., save original :=
checkVSCReadiness and defer restoring it) around the test case where function is
replaced; reference the table entry fields expectErr and function, and the
global symbol checkVSCReadiness (and TestCheckVSCReadiness) when making these
changes.
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.77.0 to 1.79.3. - [Release notes](https://github.com/grpc/grpc-go/releases) - [Commits](grpc/grpc-go@v1.77.0...v1.79.3) --- updated-dependencies: - dependency-name: google.golang.org/grpc dependency-version: 1.79.3 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
…modules/google.golang.org/grpc-1.79.3 Bump google.golang.org/grpc from 1.77.0 to 1.79.3
…NodeAgentConfig_e2e_error [main][cherry-pick] Compare affinity by string instead of exactly same compare.
Signed-off-by: lou <alex1988@outlook.com> Co-authored-by: lou <alex1988@outlook.com>
Signed-off-by: Lyndon-Li <lyonghui@vmware.com>
1. Skip deleting the restore files from storage if the backup/BSL is not found 2. Allow deleting the restore files from storage even though the BSL is readonly Signed-off-by: Wenkai Yin(尹文开) <yinw@vmware.com>
Add PSA audit and warn labels. Signed-off-by: Xun Jiang <jxun@vmware.com>
add UBI dockerfiles Use numeric user for velero-restic-restore-helper Enable multiarch builds (openshift#135) Use arm64-graviton2 for arm builds (openshift#137) Add required keys for arm builds (openshift#139) Update Travis build job to work w/o changes on new branches Use a full VM for arm Use numeric non-root user for nonroot SCC compatibility
(cherry picked from commit ccb545f) Update PR-BZ automation mapping (openshift#84) (cherry picked from commit aa2b019) Update PR-BZ automation (openshift#92) Co-authored-by: Rayford Johnson <rjohnson@redhat.com> (cherry picked from commit ecc563f) Add publish workflow (openshift#108) (cherry picked from commit f87b779)
Code-gen no longer required on verify due to vmware-tanzu#6039 Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com> oadp-1.2: Update Makefile.prow to velero-restore-helper
…nshift#280) Signed-off-by: Scott Seago <sseago@redhat.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>
* fix: ARM images Signed-off-by: Mateus Oliveira <msouzaol@redhat.com> * fixup! fix: ARM images Signed-off-by: Mateus Oliveira <msouzaol@redhat.com> --------- Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>
…#336) Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
…openshift#334) (openshift#338) add missing unit test for kopia hashing algo (openshift#337) Introduction of downstream only option to override Kopia default: - hashing algorithm - splitting algorithm - encryption algorithm With introduction of 3 environment variables it is possible to override Kopia algorithms used by Velero: KOPIA_HASHING_ALGORITHM KOPIA_SPLITTER_ALGORITHM KOPIA_ENCRYPTION_ALGORITHM If the env algorithms are not set or they are not within Kopia SupportedAlgorithms, the default algorithm will be used. This behavior is consistent with current behavior without this change. Signed-off-by: Michal Pryc <mpryc@redhat.com> Signed-off-by: Shubham Pampattiwar <shubhampampattiwar7@gmail.com>
The rework of Makefile to make it more readable and inclusion of lint as a target as well extract golangci-lint version from the upstream Dockerfile, so we test in PROW or locally on the same version as upstream. Signed-off-by: Michal Pryc <mpryc@redhat.com>
This fixes the PR openshift#334 where one additional line was in the code. This was not exposed previously as we did not had downstream CI Lint jobs. Signed-off-by: Michal Pryc <mpryc@redhat.com>
* run oadp-operator e2e test from the velero repo execute openshift/oadp-operator e2e tests directly against the velero repo locally or via prow ci Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> * update variable names, add a cleanup * make sure env variable overrides default velero_image Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> * add options to build, push, and only test Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> * add arch to name Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> * remove duplicated clean/rm operator checkout * simplify by dropping export var and use a oneliner Co-authored-by: Tiger Kaovilai <passawit.kaovilai@gmail.com> * drop export and use oneliner Co-authored-by: Tiger Kaovilai <passawit.kaovilai@gmail.com> * just in case, allow oadp to be deployed from makefile Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> * Update Makefile.prow Co-authored-by: Tiger Kaovilai <passawit.kaovilai@gmail.com> --------- Signed-off-by: Wesley Hayutin <weshayutin@gmail.com> Co-authored-by: Tiger Kaovilai <passawit.kaovilai@gmail.com>
…t#436) Signed-off-by: Scott Seago <sseago@redhat.com>
Fixes linting error. Signed-off-by: oadp-team-rebase-bot <oadp-maintainers@redhat.com>
Fix golangci-lint version extraction and disable concat-loop check. Signed-off-by: Michal Pryc <mpryc@redhat.com>
oadp-rebasebot-app
bot
changed the title
oadp-rebasebot-cloner
bot
force-pushed
the
rebase-bot-oadp-dev
branch
from
26e68c0 to
d4ea94a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/nodeagentconfig/node-agent-config.go (1)
356-362: Make this helper non-mutating.
popFromMapdeletes fromn.nodeAgentConfigs.LoadAffinity[*].NodeSelector.MatchLabelswhile the test is only trying to read an expectation. That leaves the fixture in a different state than the config that was installed and makes later reuse harder to reason about. Returning the first entry withoutdelete(or ranging inline) avoids the hidden side effect.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."Suggested change
-func popFromMap[K comparable, V any](m map[K]V) (k K, v V, ok bool) { +func firstEntryFromMap[K comparable, V any](m map[K]V) (k K, v V, ok bool) { for key, val := range m { - delete(m, key) return key, val, true } return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/nodeagentconfig/node-agent-config.go` around lines 356 - 362, The helper popFromMap mutates the input map by calling delete, which causes tests to alter fixture state; change popFromMap so it only reads and returns the first key/value without deleting (e.g., iterate and return key,val,true or range inline) and keep the same signature (popFromMap[K comparable, V any](m map[K]V) (k K, v V, ok bool)) so callers that expect a non-empty entry still work but no longer observe side effects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/nodeagentconfig/node-agent-config.go`:
- Around line 242-248: The test currently only checks that expectedLabelKey
appears as a substring in backupPodList.Items[0].Spec.Affinity.String(), which
misses incorrect values; instead, extract the structured NodeAffinity from
backupPodList.Items[0].Spec.Affinity.NodeAffinity and assert that its
RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms contain a
MatchExpressions/MatchFields or MatchLabels entry matching both the expected key
and value(s) from n.nodeAgentConfigs.LoadAffinity[1].NodeSelector.MatchLabels
(use the same helper you already have for structured checking rather than
popFromMap+String), and replicate the same structured-check logic in Restore()
so both paths validate exact key:value presence in the NodeAffinity terms.
---
Nitpick comments:
In `@test/e2e/nodeagentconfig/node-agent-config.go`:
- Around line 356-362: The helper popFromMap mutates the input map by calling
delete, which causes tests to alter fixture state; change popFromMap so it only
reads and returns the first key/value without deleting (e.g., iterate and return
key,val,true or range inline) and keep the same signature (popFromMap[K
comparable, V any](m map[K]V) (k K, v V, ok bool)) so callers that expect a
non-empty entry still work but no longer observe side effects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b399768f-7abb-4dcb-8bae-f0d8b246c483
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
changelogs/unreleased/9581-shubham-pampattiwargo.modinternal/delete/actions/csi/volumesnapshotcontent_action.gointernal/delete/actions/csi/volumesnapshotcontent_action_test.gosite/algolia-crawler.jsonsite/content/docs/main/api-types/schedule.mdsite/layouts/docs/docs.htmlsite/layouts/partials/head-docs.htmltest/e2e/basic/restore_exec_hooks.gotest/e2e/e2e_suite_test.gotest/e2e/nodeagentconfig/node-agent-config.gotest/e2e/resource-filtering/wildcard_namespaces.go
💤 Files with no reviewable changes (3)
- site/layouts/docs/docs.html
- site/algolia-crawler.json
- site/layouts/partials/head-docs.html
✅ Files skipped from review due to trivial changes (5)
- changelogs/unreleased/9581-shubham-pampattiwar
- site/content/docs/main/api-types/schedule.md
- internal/delete/actions/csi/volumesnapshotcontent_action.go
- go.mod
- test/e2e/e2e_suite_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/resource-filtering/wildcard_namespaces.go
- internal/delete/actions/csi/volumesnapshotcontent_action_test.go
Comment on lines
+242
to
+248
| expectedLabelKey, _, ok := popFromMap(n.nodeAgentConfigs.LoadAffinity[1].NodeSelector.MatchLabels) | ||
| Expect(ok).To(BeTrue(), "Expected LoadAffinity's MatchLabels should at least have one key-value pair") | ||
|
|
||
| Expect(backupPodList.Items[0].Spec.Affinity).To(Equal(expectedAffinity)) | ||
| // From 1.18.1, Velero adds some default affinity in the backup/restore pod, | ||
| // so we can't directly compare the whole affinity, | ||
| // but we can verify if the expected affinity is contained in the pod affinity. | ||
| Expect(backupPodList.Items[0].Spec.Affinity.String()).To(ContainSubstring(expectedLabelKey)) |
There was a problem hiding this comment.
Don't reduce the affinity check to a label-key substring.
Line 248 and Line 329 now pass as long as one key name appears somewhere in the formatted affinity. That misses wrong selector values or expressions—for example, kubernetes.io/arch=arm64 would still satisfy the assertion. Please inspect the structured NodeAffinity and verify the expected MatchLabels key/value pair(s) are present instead of string-matching the whole object.
Suggested direction
- expectedLabelKey, _, ok := popFromMap(n.nodeAgentConfigs.LoadAffinity[1].NodeSelector.MatchLabels)
- Expect(ok).To(BeTrue(), "Expected LoadAffinity's MatchLabels should at least have one key-value pair")
- Expect(backupPodList.Items[0].Spec.Affinity.String()).To(ContainSubstring(expectedLabelKey))
+ expectedMatchLabels := n.nodeAgentConfigs.LoadAffinity[1].NodeSelector.MatchLabels
+ Expect(expectedMatchLabels).NotTo(BeEmpty(), "expected LoadAffinity.MatchLabels to contain at least one entry")
+ Expect(affinityContainsMatchLabels(backupPodList.Items[0].Spec.Affinity, expectedMatchLabels)).To(BeTrue())Apply the same helper in Restore() so both paths validate the actual NodeAffinity terms, including label values.
Also applies to: 323-329
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/nodeagentconfig/node-agent-config.go` around lines 242 - 248, The
test currently only checks that expectedLabelKey appears as a substring in
backupPodList.Items[0].Spec.Affinity.String(), which misses incorrect values;
instead, extract the structured NodeAffinity from
backupPodList.Items[0].Spec.Affinity.NodeAffinity and assert that its
RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms contain a
MatchExpressions/MatchFields or MatchLabels entry matching both the expected key
and value(s) from n.nodeAgentConfigs.LoadAffinity[1].NodeSelector.MatchLabels
(use the same helper you already have for structured checking rather than
popFromMap+String), and replicate the same structured-check logic in Restore()
so both paths validate exact key:value presence in the NodeAffinity terms.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
15 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Documentation
itemOperationTimeoutfield to Schedule API type documentation.Tests